Fix authentication #17
Conversation
Refactors the try-catch block in the `initiateEmailSignIn` method to improve robustness and clarity. - The generic `catch` block now includes the stack trace in the log for better error diagnostics. - Adds a comment to the `on HtHttpException` block to clarify that it correctly propagates specific exceptions like `ForbiddenException`, preventing them from being masked as generic 50
Refactors the try-catch block in the `completeEmailSignIn` method to correctly handle exceptions during user lookup and creation. Previously, any `HtHttpException` (including `ForbiddenException` thrown for users without dashboard permissions) was caught and re-thrown as a generic `OperationFailedException`. This masked the original error and prevented the `errorHandler` middleware from returning the correct 403 status code. This change ensures that `HtHttpException` subtypes are re-thrown directly, allowing for proper error handling and correct HTTP responses, thus fixing the authentication vulnerability.
Refactors the parsing of the `isDashboardLogin` parameter in the `/api/v1/auth/request-code` route handler. Previously, the handler would crash with a type cast error if the `isDashboardLogin` value was sent as a string (e.g., `"true"`) instead of a boolean. This change introduces robust parsing that correctly handles values that are boolean `true`, the string `"true"`, or missing, preventing the 500 Internal Server Error and making the endpoint more resilient to varied client implementations.
Refactors the validation logic in `AuthService.initiateEmailSignIn` to correct a critical security flaw. Previously, the logic allowed verification codes to be sent to any email address during a dashboard login attempt, even if the user did not exist or lacked permissions. This change restructures the validation into a more explicit `if-else if` block. It now correctly throws an `UnauthorizedException` if the user is not found or a `ForbiddenException` if they lack permissions, ensuring that execution is halted immediately and a code is never sent for an invalid dashboard login attempt.
- Added email verification for dashboard login - Improved error handling for email mismatch - Enhanced logging for security issues - Added more informative error messages - Fixed potential security vulnerability
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @fulleni, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces several key improvements to the authentication system. It refines the dashboard login validation logic, implements a critical security check to prevent email mismatches during login verification, and enhances error handling with more detailed logging. Furthermore, it improves the flexibility of the request-code API endpoint by making its isDashboardLogin parameter parsing more robust.
Highlights
- Authentication Logic Refinement: Refactored the dashboard login validation in
AuthServiceto combine user existence and permission checks into a single, clearerif/else ifblock. This ensures that permission checks are only performed for existing users and improves the logical flow of the authentication process. - Critical Security Enhancement: Implemented a new security check in
AuthServiceduring dashboard login code verification. This check explicitly verifies that the email associated with the found user matches the email provided in the request, addressing a potential loophole and preventing unauthorized access. - Robust Error Handling & Logging: Enhanced error handling in
AuthServiceby modifyingcatchblocks to capture and log stack traces for unexpected exceptions. This provides more comprehensive debugging information. Additionally, comments were added to clarify the intentional propagation of specificHtHttpExceptioninstances. - API Request Parsing Improvement: Improved the parsing of the
isDashboardLoginflag in therequest-code.dartendpoint. The API now robustly handles both booleantrueand string literal"true"values, preventing type casting errors and making the API more resilient to varied client inputs.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes to the authentication flow. Key changes include making the parsing of the isDashboardLogin flag more robust, improving error handling by rethrowing specific exception types and logging stack traces for unexpected errors, and adding a critical security check to verify email consistency during dashboard login.
My review includes two suggestions:
- Making the new email validation check case-insensitive to prevent potential login failures for legitimate users.
- Refactoring the boolean parsing logic in
request-code.dartto use a more concise and modernswitchexpression.
Overall, these are excellent improvements to the security and stability of the authentication service.
| @@ -166,13 +166,25 @@ class AuthService { | |||
| // This closes the loophole where a non-admin user could request a code | |||
| // via the app flow and then use it to log into the dashboard. | |||
| if (isDashboardLogin) { | |||
| if (user.email != email) { | |||
There was a problem hiding this comment.
This is a great security check. However, email addresses are typically treated as case-insensitive. This comparison is case-sensitive. If the user lookup in _findUserByEmail is or becomes case-insensitive, a user logging in with [email protected] might be found in the database as [email protected], causing this check to fail incorrectly. To make this more robust, consider performing a case-insensitive comparison.
if (user.email.toLowerCase() != email.toLowerCase()) {
| final isDashboardLoginRaw = body['isDashboardLogin']; | ||
| var isDashboardLogin = false; | ||
| if (isDashboardLoginRaw is bool) { | ||
| isDashboardLogin = isDashboardLoginRaw; | ||
| } else if (isDashboardLoginRaw is String) { | ||
| isDashboardLogin = isDashboardLoginRaw.toLowerCase() == 'true'; | ||
| } |
There was a problem hiding this comment.
This logic is much more robust than the previous implementation. For improved conciseness and to leverage modern Dart features, you could consider using a switch expression. This also allows isDashboardLogin to be declared as final, which is generally preferred over var when the variable is not reassigned.
final isDashboardLoginRaw = body['isDashboardLogin'];
final isDashboardLogin = switch (isDashboardLoginRaw) {
final bool b => b,
final String s => s.toLowerCase() == 'true',
_ => false,
};
Status
READY/IN DEVELOPMENT/HOLD
Description
Type of Change